Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

core: avoid modification of accountSet cache in tx_pool #21159

Merged
merged 2 commits into from
Aug 4, 2020
Merged

core: avoid modification of accountSet cache in tx_pool #21159

merged 2 commits into from
Aug 4, 2020

Conversation

duanhao0814
Copy link
Contributor

when runReorg, we may copy the dirtyAccounts' accountSet cache to promoteAddrs
in which accounts will be promoted, however, if we have reset request at the
same time, we may reuse promoteAddrs and modify the cache content which is
against the original intention of accountSet cache. So, we need to make a new
slice here to avoid modify accountSet cache.

@fjl
Copy link
Contributor

fjl commented Jun 2, 2020 •
edited
Loading

This change looks OK, but I would like to know if the re-use of the cache causes a bug. If there is a bug in txpool which is fixed by this change, we should add a test for that.

@duanhao0814
Copy link
Contributor Author

As I see, re-use of this cache may not cause a bug here because accountSet.cache is only used as a return value in function flatten() and the function flatten() will be called in two situations:

  1. used in runReorg as we see above, in which, dirtyAccounts is declared in scheduleReorgLoop and reset to nil directly after we call scheduleReorgLoop:

    go-ethereum/core/tx_pool.go

    Lines 961 to 967 in 3f649d4

    go pool.runReorg(nextDone, reset, dirtyAccounts, queuedEvents)
    // Prepare everything for the next round of reorg
    curDone, nextDone = nextDone, make(chan struct{})
    launchNextRun = false
    reset, dirtyAccounts = nil, nil

    which means we will not call flatten() twice for one dirtyAccounts object, so modification to its cache may not cause a bug
  2. used in the exported function TxPool.Locals, the only use of TxPool.Locals will not modify the cache so this is safe:

    go-ethereum/miner/worker.go

    Lines 952 to 957 in 3f649d4

    for _, account := range w.eth.TxPool().Locals() {
    if txs := remoteTxs[account]; len(txs) > 0 {
    delete(remoteTxs, account)
    localTxs[account] = txs
    }
    }

karalabe
karalabe reviewed Jun 16, 2020
View reviewed changes
core/tx_pool.go Outdated
@@ -1032,8 +1032,8 @@ func (pool *TxPool) runReorg(done chan struct{}, reset *txpoolResetRequest, dirt
delete(events, addr)
}
}
// Reset needs promote for all addresses
promoteAddrs = promoteAddrs[:0]
// make a capable slice to avoid modify accountSet cache.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old comment was referring to the logical flow. Please add that back. The new comments seems to be just a text-explanation of the line below, which doesn't make much sense. Please revert that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@holiman
Copy link
Contributor

holiman commented Jun 18, 2020

This looks a bit odd to me: https://github.com/ethereum/go-ethereum/pull/21159/files#diff-8fecce9bb4c486ebc22226cf681416e2R1020-R1022

Basically, if reset != nil, we're going to discard the result from flatten. So why do we even call it? Couldn't we just

	var promoteAddrs []common.Address
	if dirtyAccounts != nil && reset != nil {
		promoteAddrs = dirtyAccounts.flatten()
	}
	pool.mu.Lock()
	if reset != nil {

Not sure if I'm missing some subtle side-effect

@duanhao0814
Copy link
Contributor Author

This looks a bit odd to me: https://github.com/ethereum/go-ethereum/pull/21159/files#diff-8fecce9bb4c486ebc22226cf681416e2R1020-R1022

Basically, if reset != nil, we're going to discard the result from flatten. So why do we even call it? Couldn't we just

	var promoteAddrs []common.Address
	if dirtyAccounts != nil && reset != nil {
		promoteAddrs = dirtyAccounts.flatten()
	}
	pool.mu.Lock()
	if reset != nil {

Not sure if I'm missing some subtle side-effect

Yes! I agree that if reset != nil, we even don't need to call flatten at all.
And also, when reset != nil, we can allocate an empty slice with capable size.

@duanhao0814
core: avoid modification of accountSet cache in tx_pool
0b748ef 
when runReorg, we may copy the dirtyAccounts' accountSet cache to promoteAddrs
in which accounts will be promoted, however, if we have reset request at the
same time, we may reuse promoteAddrs and modify the cache content which is
against the original intention of accountSet cache. So, we need to make a new
slice here to avoid modify accountSet cache.
holiman
holiman approved these changes Aug 3, 2020
View reviewed changes
Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

holiman
holiman reviewed Aug 4, 2020
View reviewed changes
core/tx_pool.go Outdated
@@ -1023,7 +1023,8 @@ func (pool *TxPool) runReorg(done chan struct{}, reset *txpoolResetRequest, dirt
defer close(done)

var promoteAddrs []common.Address
if dirtyAccounts != nil {
if dirtyAccounts != nil && reset != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be reset == nil, right?

@holiman holiman merged commit b35e4fc into ethereum:master Aug 4, 2020
@holiman holiman added this to the 1.9.19 milestone Aug 4, 2020
@fjl fjl removed the pr:merge label Aug 4, 2020
This was referenced Feb 24, 2021
Closed
Merged
enriquefynn pushed a commit to enriquefynn/go-ethereum that referenced this pull request Mar 10, 2021
@duanhao0814 @fjl @enriquefynn
core: avoid modification of accountSet cache in tx_pool (ethereum#21159)
3701e40 
* core: avoid modification of accountSet cache in tx_pool

when runReorg, we may copy the dirtyAccounts' accountSet cache to promoteAddrs
in which accounts will be promoted, however, if we have reset request at the
same time, we may reuse promoteAddrs and modify the cache content which is
against the original intention of accountSet cache. So, we need to make a new
slice here to avoid modify accountSet cache.

* core: fix flatten condition + comment

Co-authored-by: Felix Lange <fjl@twurst.com>
@gzliudan gzliudan mentioned this pull request May 13, 2024
Merged
20 tasks
gzliudan pushed a commit to gzliudan/XDPoSChain that referenced this pull request May 13, 2024
@duanhao0814 @fjl @gzliudan
core: avoid modification of accountSet cache in tx_pool (ethereum#21159)
28baf9f 
* core: avoid modification of accountSet cache in tx_pool

when runReorg, we may copy the dirtyAccounts' accountSet cache to promoteAddrs
in which accounts will be promoted, however, if we have reset request at the
same time, we may reuse promoteAddrs and modify the cache content which is
against the original intention of accountSet cache. So, we need to make a new
slice here to avoid modify accountSet cache.

* core: fix flatten condition + comment

Co-authored-by: Felix Lange <fjl@twurst.com>
@gzliudan gzliudan mentioned this pull request May 17, 2024
Merged
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@karalabe karalabe karalabe left review comments

@holiman holiman holiman approved these changes

@rjl493456442 rjl493456442 Awaiting requested review from rjl493456442 rjl493456442 is a code owner

Assignees

@fjl fjl

Labels
None yet
Projects
None yet
Milestone
1.9.19
Development

Successfully merging this pull request may close these issues.
5 participants
@duanhao0814 @fjl @holiman @karalabe @adamschmideg